Skip to content

fix: fix finfo guesser in coroutines#376

Merged
binaryfire merged 9 commits into0.4from
hotfix/fix-finfo-guesser-coroutine
May 1, 2026
Merged

fix: fix finfo guesser in coroutines#376
binaryfire merged 9 commits into0.4from
hotfix/fix-finfo-guesser-coroutine

Conversation

@albertcht
Copy link
Copy Markdown
Member

Problem

FileinfoMimeTypeGuesser cached finfo instances in a static class property($finfoCache). In a coroutine environment this is unsafe: a single finfo resource held in a static array is shared across all coroutines, which can lead to race conditions and undefined behavior when multiple coroutines concurrently call guessMimeType().

Solution

Replace the static array cache with Context::getOrSet(), which stores the finfo instance in Hypervel's coroutine context. Each coroutine gets its own isolated finfo instance, eliminating the shared-state problem while preserving the caching benefit within a single coroutine's lifetime.

The cache key is built from a fixed prefix plus the optional magic file path, matching the previous per-magic-file granularity:

// Before
private static $finfoCache = [];
$finfo = self::$finfoCache[$this->magicFile] ??= new finfo(FILEINFO_MIME_TYPE, $this->magicFile);

// After
private const FINFO_CACHE_KEY = '__support.finfo_mime_type_guesser.';
$finfo = Context::getOrSet(
    self::FINFO_CACHE_KEY . ($this->magicFile ?? ''),
    fn () => new finfo(FILEINFO_MIME_TYPE, $this->magicFile)
);

@albertcht albertcht added the bug Something isn't working label Apr 28, 2026
@albertcht
Copy link
Copy Markdown
Member Author

@copilot resolve the merge conflicts in this pull request

Copilot AI and others added 2 commits April 28, 2026 07:37
…er-coroutine

# Conflicts:
#	src/foundation/src/Bus/PendingChain.php
#	src/foundation/src/Support/Providers/RouteServiceProvider.php
#	src/http/src/Client/PendingRequest.php
#	src/http/src/Client/Request.php
#	src/sentry/config/sentry.php
#	src/telescope/config/telescope.php

Co-authored-by: albertcht <9117929+albertcht@users.noreply.github.com>
Co-authored-by: albertcht <9117929+albertcht@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 28, 2026

@copilot resolve the merge conflicts in this pull request

Resolved in 7c0e377. The branch was based on main instead of 0.4, so I merged origin/0.4 into the PR branch and resolved the 7 content conflicts by accepting the 0.4 versions for the unrelated files (from phpstan/cs-fixer commits). The PR diff now cleanly shows only the intended changes: FileinfoMimeTypeGuesser.php, the test file, and the test fixture.

@albertcht albertcht self-assigned this Apr 28, 2026
@albertcht albertcht requested a review from binaryfire April 28, 2026 07:43
binaryfire added 4 commits May 1, 2026 11:47
The previous fix imported `Hypervel\Context\Context`, which does not exist
in this codebase — `hypervel/context` only exposes `CoroutineContext`,
`RequestContext`, `ResponseContext`, `ParentCoroutineContext`,
`ReplicableContext`. Calling `guessMimeType()` threw a class-not-found
error.

Switch to `CoroutineContext::getOrSet()` to match how every other ported
package uses per-coroutine caching (JwtGuard, Sentry\Hub, UrlGenerator,
Log\Context\Repository).

Rename the constant to `FINFO_CONTEXT_KEY_PREFIX` per CLAUDE.md naming
convention (existing examples: `CACHED_SCHEME_CONTEXT_KEY`,
`CACHED_ROOT_CONTEXT_KEY`) and promote it to `public` so the regression
test can read the cached `finfo` back out of context.
…tions

- Extend `Hypervel\Tests\TestCase` instead of raw `PHPUnit\Framework\TestCase`
- Drop the `RunTestsInCoroutine` trait (already on the base class)
- Drop `@internal` / `@coversNothing` docblock and `: void` return types
- Update fixture path to `Fixtures/` (capital F)

Replace the bare `Coroutine::create()` regression test with `parallel()`,
which awaits the children and propagates assertion failures back to
PHPUnit. The previous form spawned 5 coroutines without a `WaitGroup`,
so failures inside them never failed the test.

Strengthen the test to actually exercise the per-coroutine isolation
the fix provides: each coroutine pulls its cached `finfo` back out of
`CoroutineContext` via `FINFO_CONTEXT_KEY_PREFIX` and the test asserts
all 5 instances are distinct objects. Reverting to the shared static
cache would now fail this test.
@binaryfire
Copy link
Copy Markdown
Collaborator

@albertcht LGTM. I updated the namespaces from 0.3 -> 0.4 (CoroutineContext).

I also moved fixtures to Fixtures. I've standardised everything in 0.4 to use Fixtures (with a capital F) so the dir works for both class files and non-class files. Laravel and Hypervel 0.3 use a random mix of dir names, which gets a bit messy:

  • stubs
  • Stub
  • Stubs
  • fixtures
  • Fixtures

@binaryfire binaryfire merged commit e4b991a into 0.4 May 1, 2026
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants